Add lark sheets connector #17218

7c00 · 2022-01-24T16:23:32Z

This pull introduces Lark Sheets Connector, which is inspired by Google Sheets Connector. Though there are tiny differences between them.

Developers could run LarkSheetsQueryRunners#main, and then follow the QueryModel section from the doc to try the connector.

Test plan

Integation test
Unit test

== RELEASE NOTES ==

Connectors
* Introduce Lark sheets connector.

7c00 · 2022-01-27T02:52:00Z

Hello, @ChunxuTang @beinan. Could you help review this PR?

rohanpednekar · 2022-01-27T18:25:24Z

Thanks, @7c00 for your contribution, we were planning to request @ChunxuTang and @beinan as they helped us earlier to review the google sheet connector.

ChunxuTang

Hi @7c00, thanks a lot for your work! This looks to be a very nice feature, and the Presto community could benefit.
Just left some suggestions/comments. For the first round of review, I mainly focused on the coding styles. Thanks again for your contributions!

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/SheetsMetadata.java

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/SheetsRecordCursor.java

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/SheetsSplit.java

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/api/SheetsApiFactory.java

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/SheetsConfig.java

ChunxuTang · 2022-02-05T03:04:45Z

presto-lark-sheets/src/test/resources/testing-app-secret-base64

@@ -0,0 +1 @@
+ewogICJhcHAtc2VjcmV0IiA6ICJYSHdrckVna0N6Y3dFZ0NDNlpQQjRnWXZtb25OSmdNTiIKfQ==


Is there a TTL for this secret? And is there an owner email account? There's a potential risk here that when the secret expires, it may break presto CI workflows.

There is no TTL for the secret. The secret is not bound to an email account.
I wonder if we should disable TestSheetsIntegration by default as TestGoogleSheets.

Yeah, I think we may need to disable it.
Hi @beinan, could you help disable this integration test by default?

Yes, let's disable the test, otherwise it would be too flaky I guess

We could use @Test(enabled = false) in the integration test.

Disabled all the tests in TestLarkSheetsIntegration.

7c00 · 2022-02-07T10:26:44Z

@ChunxuTang Thanks a lot for your comments. I haved update the code. Could you please review it again?

ChunxuTang

Hi @7c00, thanks for your revision!
Left a few minor suggestions/comments.

ChunxuTang · 2022-02-11T17:52:11Z

presto-docs/src/main/sphinx/connector/larksheets.rst

+
+    connector.name=lark-sheets
+    app-domain=FEISHU
+    app-id=cli_a16fc7ddf4b9900d


Just wondering is this a real app id?

Yes, it is a real app id.

Just wondering as this is a real ID, is it OK to expose it in an example file? Maybe just use exampleAppId?
Your call.

Updated to a fake app id as example_app_id.

presto-docs/src/main/sphinx/connector/larksheets.rst

ChunxuTang · 2022-02-11T17:55:14Z

presto-docs/src/main/sphinx/connector/larksheets.rst

+
+The ``token`` property is taken from the last path segment of the spreadsheet url, e.g.
+``shtcnBf5pg4BNSkwV2Ku5xwW9Pf`` for
+``https://test-ch80md45anra.feishu.cn/sheets/shtcnBf5pg4BNSkwV2Ku5xwW9Pf?sheet=MT1p4I``


Looks like this is a real spreadsheet. Will the link expire?

No, it will not expire.

To make it more readable, I might use a pattern string rather than a real link. e.g. https://{account_id}.feishu.ch/sheet/{token}/sheet={sheet_id}

Added the pattern string. And give an example as well.

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/SheetsMetadata.java

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/SheetsRecordCursor.java

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/api/SheetsApiFactory.java

ChunxuTang · 2022-02-11T18:38:58Z

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/api/SheetsApiFactory.java

+    public class SimpleSheetsApi
+            implements SheetsApi


Just wondering as the CachingSheetsApi is implemented in a separate file, why do you prefer to implement the SimpleSheetsApi in the factory class? Any specific reasons?

Possible some of my perference during development I guess. Have rafactored SimpleSheetsApi to a top level class to make it matched the code style of CachingSheetsApi.

presto-lark-sheets/src/test/java/com/facebook/presto/lark/sheets/TestSheetsPlugin.java

ChunxuTang · 2022-02-11T18:48:48Z

presto-lark-sheets/src/test/java/com/facebook/presto/lark/sheets/TestSheetsUtil.java

@@ -0,0 +1,70 @@
+/*


Very minor suggestion: As you added some utility functions to SheetUtil, would you mind also testing them here?

Sure. Added tests for new functions here.

Did you add them? Forgot to commit changes?
It doesn't show any changes here.

The tests was added to another class by mistake. Have corrected this and moved the test to TestLarkSheetsUtil.

7c00 · 2022-02-16T11:38:40Z

@ChunxuTang Thanks for your comments. ❤️
Have updated the code. Could you please take a review again? Thanks in advance.

ChunxuTang · 2022-02-17T22:13:34Z

@ChunxuTang Thanks for your comments. ❤️ Have updated the code. Could you please take a review again? Thanks in advance.

Sure! I'll take another round of review very soon this week.

ChunxuTang

@7c00 Thanks for your contributions!

As you created this PR some time ago and it is now left a bit behind the master branch, could you merge master into this feature branch to avoid conflicts?

beinan · 2022-02-21T05:22:41Z

presto-docs/src/main/sphinx/connector/larksheets.rst

+
+The ``token`` property is taken from the last path segment of the spreadsheet url, e.g.
+``shtcnBf5pg4BNSkwV2Ku5xwW9Pf`` for
+``https://test-ch80md45anra.feishu.cn/sheets/shtcnBf5pg4BNSkwV2Ku5xwW9Pf?sheet=MT1p4I``


To make it more readable, I might use a pattern string rather than a real link. e.g. https://{account_id}.feishu.ch/sheet/{token}/sheet={sheet_id}

beinan · 2022-02-21T05:27:47Z

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/SheetsConnector.java

+
+import static java.util.Objects.requireNonNull;
+
+public class SheetsConnector


To be more consistent, can we use LarkSheetsConnector as the class name? Similarly, other class names should also use LarkSheets

Fine. Renamed to LarkSheetsConnector and prefixed other class with Lark as well.

beinan · 2022-02-21T05:39:46Z

presto-lark-sheets/src/test/resources/testing-app-secret-base64

@@ -0,0 +1 @@
+ewogICJhcHAtc2VjcmV0IiA6ICJYSHdrckVna0N6Y3dFZ0NDNlpQQjRnWXZtb25OSmdNTiIKfQ==


Yes, let's disable the test, otherwise it would be too flaky I guess

We could use @Test(enabled = false) in the integration test.

7c00 · 2022-02-21T12:23:47Z

@ChunxuTang @beinan Thanks for your comments. I have rebased my branch with master and updated the code as the comments. Could you please review it again? Thanks in advance.

beinan

just left a couple comments for some minor issues, I will continue review this giant pr in this week. Many thanks for this create contribution!

beinan · 2022-02-22T07:45:13Z

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/LarkSheetsConnector.java

+        LarkSheetsApi api = apiFactory.get();
+        LarkSheetsApi cachingApi = new CachingLarkSheetsApi(api);


nit: inline api?

Suggested change

LarkSheetsApi api = apiFactory.get();

LarkSheetsApi cachingApi = new CachingLarkSheetsApi(api);

LarkSheetsApi cachingApi = new CachingLarkSheetsApi(apiFactory.get());

beinan · 2022-02-22T07:48:30Z

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/LarkSheetsErrorCode.java

+    SCHEMA_TOKEN_NOT_PROVIDED(0x03, USER_ERROR),
+    SCHEMA_NOT_EXISTS(0x04, USER_ERROR),
+    SHEET_NAME_AMBIGUOUS(0x04, USER_ERROR),
+    SHEETS_API_ERROR(0x10, EXTERNAL),


Why is this plural? if the plural is necessary, it might be better to follow alphabetical order

Maybe a typo. Have reorderred the error codes to three catagories.

beinan · 2022-02-22T07:50:28Z

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/LarkSheetsErrorCode.java

+    SCHEMA_ALREADY_EXISTS(0x03, USER_ERROR),
+    SCHEMA_TOKEN_NOT_PROVIDED(0x03, USER_ERROR),
+    SCHEMA_NOT_EXISTS(0x04, USER_ERROR),
+    SHEET_NAME_AMBIGUOUS(0x04, USER_ERROR),


why some of the error are sharing the same error code? the 0x03 and 0x04 are used two times here.

Maybe a typo. Have reorderred the error codes to three catagories.

beinan · 2022-02-22T07:53:30Z

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/LarkSheetsMetadata.java

+        }
+        else if (matched.size() > 1) {
+            throw new PrestoException(LarkSheetsErrorCode.SHEET_NAME_AMBIGUOUS,
+                    format("Ambiguous name %s in spreadsheet %s: matched sheets: %s", sheetName, token, matched));


can we throw an exception with the token? will there be any security concern?

Putting the raw tokens in exception messages might cause data leak. Have masked the token before writing the token.

beinan · 2022-02-22T07:56:25Z

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/LarkSheetsMetadata.java

+    public List<SchemaTableName> listTables(ConnectorSession session, Optional<String> schemaName)
+    {
+        if (!schemaName.isPresent()) {
+            throw new PrestoException(NOT_PERMITTED, "Schema is required to list tables");


nit: any default schema for lark/feishu?

Default schema is not supported, since we dont know which spreadsheet shoud be associated with the schema.

beinan · 2022-02-22T07:58:33Z

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/LarkSheetsMetadata.java

+    public void renameSchema(ConnectorSession session, String source, String target)
+    {
+        LarkSheetsSchema schema = requireVisibleSchema(session, source);
+        checkSchemaUpdatable(schema, session.getUser(), "rename");


is it possible to use constant or enum for the operations?

The operation parameter in checkSchemaUpdatable is used for generating an error message. So I think it might be not neccessary to define constants of enums for it.

beinan · 2022-02-22T08:00:22Z

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/LarkSheetsMetadata.java

+        LinkedHashMap<String, LarkSheetsColumnHandle> columns = new LinkedHashMap<>(numColumns);
+        for (int i = 0; i < numColumns; i++) {
+            String rawColumnName = header.get(i);
+            if (rawColumnName == null) {


I'm just thinking if we could use the column index when the column name is null. Your call.

The column name can be a sequence of any UTF-8 characters. So it might be hard to define the column index name spec. For example, using $2 as the column index to reference the second column, it might conflict with another column with the name of $2. In the other hand, updating a spreadsheet to fill a column name is very easy for end users. So column index name is not be supported.

gotcha, thanks for the clarification!

7c00 · 2022-02-25T06:43:04Z

@ChunxuTang @beinan Hi, thanks for your review. ❤️ Do we need more review before merge this pr?

beinan

I think it looks good to me except two very minor issues.

beinan · 2022-02-23T22:54:26Z

presto-lark-sheets/src/main/java/com/facebook/presto/lark/sheets/LarkSheetsRecordCursor.java

+        this.api = requireNonNull(api, "api is null");
+        this.split = requireNonNull(split, "split is null");
+        this.readColumns = requireNonNull(readColumns, "columns is null");
+        this.indexMapping = createIndexMapping(readColumns);


unnecessary the ".this"

beinan · 2022-02-26T05:53:38Z

presto-lark-sheets/src/test/java/com/facebook/presto/lark/sheets/TestLarkSheetsIntegration.java

+    // Used for fast switch to enable or disable tests
+    private static final boolean TEST_ENABLED = false;
+    private static final String CATALOG = "larksheets";
+    // https://test-ch80md45anra.feishu.cn/sheets/shtcnBf5pg4BNSkwV2Ku5xwW9Pf?sheet=MT1p4I


what's this comment for?

This url points to the spreadsheet used for this integration test. The TESTING_TOKEN is extracted from the url. I have added comments to make it more clear.

7c00 · 2022-03-03T13:28:12Z

ping @beinan @ChunxuTang

ChunxuTang

LGTM. @7c00 Thanks for your nice work!

7c00 · 2022-03-07T02:59:53Z

Rebased with master branch.

beinan

lgtm, rerun the test

7c00 · 2022-03-09T12:06:47Z

Thanks @beinan ❤️ . This CI finished. Could we merge this PR?

7c00 · 2022-03-10T00:52:44Z

Thanks @beinan @ChunxuTang for your time to review!

rohanpednekar · 2022-04-25T21:17:59Z

@7c00 Do you think you can provide short documentation on how to use this connector that we can publish at https://prestodb.io/docs/current/connector.html ?

7c00 · 2022-04-26T01:57:59Z

@rohanpednekar Yes. The connector doc is included in the PR. It's https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/larksheets.rst. It's a bit strange that it's not displayed in prestodb website.

rohanpednekar · 2022-04-26T19:00:21Z

Thanks @7c00 for the confirmation, I guess this ll get published with the 0.272 release notes PR

7c00 force-pushed the presto-lark branch 4 times, most recently from c7198fd to 65ac8f6 Compare January 26, 2022 10:04

7c00 marked this pull request as ready for review January 26, 2022 10:17

7c00 changed the title ~~WIP: Add lark sheets connector~~ Add lark sheets connector Jan 26, 2022

7c00 force-pushed the presto-lark branch 2 times, most recently from 020e181 to 6f96def Compare January 27, 2022 02:08

v-jizhang self-requested a review January 27, 2022 15:07

rongrong requested a review from beinan January 27, 2022 19:19

7c00 force-pushed the presto-lark branch from 6f96def to 17e2b53 Compare January 29, 2022 07:53

rohanpednekar added the awaiting-community-help label Feb 4, 2022

ChunxuTang reviewed Feb 5, 2022

View reviewed changes

7c00 force-pushed the presto-lark branch 2 times, most recently from d1c4dd5 to 91d0063 Compare February 7, 2022 09:09

ChunxuTang reviewed Feb 11, 2022

View reviewed changes

7c00 force-pushed the presto-lark branch from 91d0063 to 48fabce Compare February 16, 2022 11:24

ChunxuTang reviewed Feb 19, 2022

View reviewed changes

7c00 force-pushed the presto-lark branch 3 times, most recently from 9866ddf to b8cd8f8 Compare February 21, 2022 03:25

beinan reviewed Feb 21, 2022

View reviewed changes

7c00 force-pushed the presto-lark branch 3 times, most recently from fb67c06 to aefb67a Compare February 21, 2022 11:53

7c00 force-pushed the presto-lark branch from aefb67a to fb2bb08 Compare February 21, 2022 11:58

7c00 requested a review from beinan February 21, 2022 12:23

beinan reviewed Feb 22, 2022

View reviewed changes

7c00 force-pushed the presto-lark branch from fb2bb08 to 06d1624 Compare February 23, 2022 13:02

7c00 requested a review from beinan February 23, 2022 13:19

beinan reviewed Feb 26, 2022

View reviewed changes

7c00 force-pushed the presto-lark branch 2 times, most recently from 08e82ab to ee0facb Compare February 28, 2022 06:32

7c00 requested a review from beinan February 28, 2022 07:42

ChunxuTang approved these changes Mar 6, 2022

View reviewed changes

7c00 force-pushed the presto-lark branch from ee0facb to 03c9a94 Compare March 7, 2022 02:58

7c00 force-pushed the presto-lark branch 2 times, most recently from 5585ba4 to 0f6088f Compare March 8, 2022 03:52

Add lark sheets connector

3f008ef

7c00 force-pushed the presto-lark branch from 0f6088f to 3f008ef Compare March 8, 2022 12:33

beinan approved these changes Mar 9, 2022

View reviewed changes

beinan merged commit 8750895 into prestodb:master Mar 9, 2022

7c00 deleted the presto-lark branch March 10, 2022 01:55

varungajjala mentioned this pull request Mar 22, 2022

Add release notes for 0.272 #17499

Closed

9 tasks

asjadsyed mentioned this pull request Mar 23, 2022

Add release notes for 0.272 #17510

Closed

9 tasks

asjadsyed mentioned this pull request Apr 1, 2022

Add release notes for 0.272 #17564

Merged

8 tasks

		@@ -0,0 +1 @@
		ewogICJhcHAtc2VjcmV0IiA6ICJYSHdrckVna0N6Y3dFZ0NDNlpQQjRnWXZtb25OSmdNTiIKfQ==


		import static java.util.Objects.requireNonNull;

		public class SheetsConnector

		LarkSheetsApi api = apiFactory.get();
		LarkSheetsApi cachingApi = new CachingLarkSheetsApi(api);

Add lark sheets connector #17218

Add lark sheets connector #17218

Conversation

7c00 commented Jan 24, 2022 • edited Loading

7c00 commented Jan 27, 2022

rohanpednekar commented Jan 27, 2022

ChunxuTang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7c00 commented Feb 7, 2022

ChunxuTang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7c00 commented Feb 16, 2022

ChunxuTang commented Feb 17, 2022

ChunxuTang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7c00 commented Feb 21, 2022

beinan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7c00 Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7c00 Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7c00 Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7c00 commented Feb 25, 2022

beinan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7c00 commented Mar 3, 2022

ChunxuTang left a comment

Choose a reason for hiding this comment

7c00 commented Mar 7, 2022

beinan left a comment

Choose a reason for hiding this comment

7c00 commented Mar 9, 2022

7c00 commented Mar 10, 2022

rohanpednekar commented Apr 25, 2022

7c00 commented Apr 26, 2022

rohanpednekar commented Apr 26, 2022

7c00 commented Jan 24, 2022 •

edited

Loading

7c00 Feb 23, 2022 •

edited

Loading

7c00 Feb 23, 2022 •

edited

Loading

7c00 Feb 23, 2022 •

edited

Loading